-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More linear algebra #1821
More linear algebra #1821
Conversation
This now has some conflicts. @joschmitt could you have a look? |
I rebased. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1821 +/- ##
==========================================
- Coverage 86.09% 86.05% -0.05%
==========================================
Files 98 98
Lines 36404 36259 -145
==========================================
- Hits 31343 31202 -141
+ Misses 5061 5057 -4 ☔ View full report in Codecov by Sentry. |
@fingolfin @thofma this needs a manual merge due to differently named CI jobs (matching branch in Hecke) |
] | ||
a = matrix(R, [1 2 3; 3 2 1; 0 0 2]) | ||
|
||
@test AbstractAlgebra.Solve.matrix_normal_form_type(R) === AbstractAlgebra.Solve.HowellFormTrait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the rhs here be NFTrait
? And if not, why can this ring have two different traits? Or is there something going on that I don't see?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's Z/nZ, so sometimes there are zero divisors and sometimes it is a field. We (Tommy) decided that we always give it the HowellFormTrait
to not make the used normal form runtime dependent.
There is still code (via flint) that can only be used in the case where n is prime, so I explicitly call this in this testset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok. Thanks for the explanation
Where is
set for Z/nZ? |
In AbstractAlgebra. It uses the generic function for rings via |
Adjust to Nemocas/AbstractAlgebra.jl#1738 and deduplicate some code.
I decided to redirect
lazy_transpose
totranspose
for the flint types because this allows us to just call the generic code in more cases.Closes #1795.